Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release-5.26] backport API changes copy and list components from upstream. #2071

Closed

Conversation

flouthoc and others added 30 commits August 4, 2023 16:27
`c/image` uses `Instance(` API to get the required details of an
instance while performing replication `Platform` of instance is needed
hence `ListUpdate` must include platform.

Needed by: containers#1987

Signed-off-by: Aditya R <[email protected]>
TryReusingBlob now contains a new option `RequiredCompression` which
filters the blob by checking against a compression of the blob which is
considerd to be resued, in case `RequiredCompression` is set and `info`
of the blob being reused does not matches, no blob is returned.

Signed-off-by: Aditya R <[email protected]>
Fixes: containers#2023 (comment).

`assert.Equal` expects `assert.Equal(t, expected, actual)` instead of `assert.Equal(t, actual, expected)`.

Ref:https://pkg.go.dev/github.com/stretchr/testify/assert#Assertions.Equal

Signed-off-by: Aditya R <[email protected]>
Setting SystemContext.ArchitectureChoice to "" does not mean "match any/the first platform";
it's the default behavior of SystemContext, and it means "choose for the current runtime
architecture". (Originally discussed in
containers#1789 (comment) )

I.e. on amd64 these two test cases are redundant with the first two instances above,
and on other architectures (notably ARM) they cause failures.

So just drop them.

Signed-off-by: Miloslav Trmač <[email protected]>
…gorithmNames

There is a need to read annotations of a particular instance to get its
compression. Expose `Annotations` as a read-only field.

Needed By: containers#1987

Signed-off-by: Aditya R <[email protected]>
- Generally I discourage unsing named return values, it's easy
  to miss that it wasn't set
- I have no idea what the "in the middle of a multi-streamed copy"
  paragraph refers to.

Signed-off-by: Miloslav Trmač <[email protected]>
... so that we don't have to carry it around in extra
parameters. Migrating individual functions will follow.

Signed-off-by: Miloslav Trmač <[email protected]>
... so that we can eliminate three parameters.

Signed-off-by: Miloslav Trmač <[email protected]>
... so that the code looks the same here and in possible callees.

Signed-off-by: Miloslav Trmač <[email protected]>
Use options.Progress and ProgressInterval directly.

Signed-off-by: Miloslav Trmač <[email protected]>
Use c.options.OciDecryptConfig directly.

Signed-off-by: Miloslav Trmač <[email protected]>
Use c.options.OciEncryptConfig directly.

Signed-off-by: Miloslav Trmač <[email protected]>
Use c.options.DownloadForeignLayers directly.

Signed-off-by: Miloslav Trmač <[email protected]>
Use ic.c.options.OciEncryptLayers directly.

Signed-off-by: Miloslav Trmač <[email protected]>
So that we don't need to pass it around in a parameter.

Signed-off-by: Miloslav Trmač <[email protected]>
Use copier.unparsedToplevel now that it exists.

Signed-off-by: Miloslav Trmač <[email protected]>
... so that we don't need to carry it around in parameters.

Signed-off-by: Miloslav Trmač <[email protected]>
It is no longer used.

Signed-off-by: Miloslav Trmač <[email protected]>
... so that we don't have so many unnamed return values, and
we can manage the return values as a batch.

Signed-off-by: Miloslav Trmač <[email protected]>
…sult on a match

... so that the caller doesn't have to assemble it. Using a pointer-or-nil eliminates
a separate boolean.

Signed-off-by: Miloslav Trmač <[email protected]>
... instead of three separate ones.

Signed-off-by: Miloslav Trmač <[email protected]>
If `UpdateCompressionAlgorithms` is set then `Annotations` map must be
initialized if its not set.

Signed-off-by: Aditya R <[email protected]>
Modifies `copy/single` to correctly use the feature from
containers#2023, that is if compression is
changed blob must be resued only if it matches required compression.

Signed-off-by: Aditya R <[email protected]>
Create a wrapper around arguments of `copySingleImage`

Signed-off-by: Aditya R <[email protected]>
After containers#2048 there is no room for copy/multiple to pass custom compressionFormat to copySingleImage
while processing each instance, following PR introduces that functionality again and wraps options
to simpler struct.

Signed-off-by: Aditya R <[email protected]>
If its a regular copy callers might not set `compressionFormat` and
`compressionLevel` in such case still honor compression which is set in
DestinationCtx from copier.

Signed-off-by: Aditya R <[email protected]>
* copy.Options now contains a new field `EnsureCompressionVariantExists
  map[string]int` which allows users to specify if they want to clone
images with specified `compression`.

Signed-off-by: Aditya R <[email protected]>
Implement `instanceCopyClone` for multiple compression.

Signed-off-by: Aditya R <[email protected]>
While performing copy, set a custom compression passed generated from
prepareInstanceCopies.

Signed-off-by: Aditya R <[email protected]>
Following option does not provides a way to detect and exclude
compression if it already exists, this feature may be implemented in
future.

See: containers#1987 (comment)

Signed-off-by: Aditya R <[email protected]>
After implementing `instanceCopyClone` now instance to be copied can
exceed the original number of instances in the source, so amend report
message to make it more meaningful.

Signed-off-by: Aditya R <[email protected]>
* test multiple copy requests of same compression

Signed-off-by: Aditya R <[email protected]>
EnsureCompressionVariantsExist is only valid when working with a
manifest list.

Signed-off-by: Aditya R <[email protected]>
When copying multiple images i.e `instanceCopyClone` and no image
exists in registry in such case if clones are prepared and
copied first then original copies will reuse blobs from the clone which
is unexpected since argument `requireCompressionFormatMatch` is by
default false for `instanceCopyCopy` case.

Problem described in following PR is easily reproduceable when working
with tools such as buildah, because without this PR buildah will push
`clones` first instead of originals and later on `originals` will reuse
blobs from `clones`.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 4, 2023

@mtrmac PTAL, I will create a bump version PR after this.

@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 4, 2023

Huh skopeo tests are failing I wonder why ? is it because I missed to backport some commit ?

Edit: Looks like skopeo test on this branch is using skopeo from upstream, which has updated deps. Not sure if we can update those deps in this backport. @mtrmac Any suggestions ?

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 4, 2023

@flouthoc On a stable branch, set SKOPEO_CI_TAG to a similarly frozen Skopeo version (a branch if there is one, or just a commit ID).

Ideally we should have done that at time of branching … I’m afraid that rarely happens.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Aug 4, 2023
@mtrmac
Copy link
Collaborator

mtrmac commented Aug 4, 2023

Code LGTM… but per the other conversation, this should be a 5.27.0. Let me work on that…

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 4, 2023

@flouthoc I have filed #2074 for what I think is necessary to make the tests pass, and #2075 as a proof of concept — note that that’s against the wrong branch.

@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 5, 2023

Okay should I close this PR and create similar PR against release-5.27 then

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 5, 2023

@flouthoc Now that CI on the 5.27 branch is running, yes, let’s do a 5.27.0.

I’ll update #2075 just so that it is ready, but you did all the backporting work so you deserve the full credit — if you want to rebase+re-file this, by all means go ahead.

@@ -6,9 +6,9 @@ env:
#### Global variables used for all tasks
####
# Name of the ultimate destination branch for this CI run
DEST_BRANCH: "main"
DEST_BRANCH: "release-5.26"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

.cirrus.yml Show resolved Hide resolved
@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 5, 2023

@mtrmac it seems just pointing #2075 to newer branch will do the trick, it will be easier for you to point changes #2075 to release-5.27. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants